Stratix 10 Quad Port Ram Support #3180
Conversation
| if (ram_info.port_a_output_clock && ram_info.port_b_output_clock) { | ||
| reg_outputs = true; //Sequential output | ||
| } else if (!ram_info.port_a_output_clock && !ram_info.port_b_output_clock) { | ||
| reg_outputs = false; //Comb output |
There was a problem hiding this comment.
Please edit the comments in your code to be consistent the coding style guide:
- Add a space after
// - Avoid using block comments for implementation-related commnets
| @@ -613,8 +625,9 @@ void generate_opname_ram (t_node* vqm_node, const LogicalModels& arch_models, st | |||
| } | |||
|
|
|||
| //A dual port memory, both port A and B params have been found | |||
| } else { | |||
| VTR_ASSERT(ram_info.mode == "dual_port" || ram_info.mode == "bidir_dual_port"); | |||
| } else if (ram_info.mode == "dual_port" || ram_info.mode == "bidir_dual_port"){ | |||
| } else { | ||
| //3b) Use the more detailed name, since it was found in the architecture | ||
| mode_hash = tmp_mode_hash; | ||
| } |
There was a problem hiding this comment.
re-write this if statement get rid of the branch where nothing is done
|
There's a failing regtest, but it may just need a golden result update. The vqm2blif output has changed, but the changes seem like block name changes. Here's a snippet: |
| @@ -568,6 +568,16 @@ void generate_opname_ram (t_node* vqm_node, const LogicalModels& arch_models, st | |||
| cout << " port A output sequential: " << bool(ram_info.port_a_output_clock) << "\n"; | |||
| cout << " port B output sequential: " << bool(ram_info.port_b_output_clock) << "\n"; | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
Suggest a comment of what the else is:
// quad port mode
| <port clock="clk_portbin" combinational_sink_ports="eccstatus portadataout portbdataout " name="portbdatain"/> | ||
| <port clock="clk_portbin" combinational_sink_ports="eccstatus portadataout portbdataout " name="portbaddr"/> | ||
| <port clock="clk_portbin" combinational_sink_ports="eccstatus portadataout portbdataout " name="portbaddr2"/> | ||
| <port clock="clk_portbin" combinational_sink_ports="eccstatus portadataout portbdataout " name="portbwe"/> |
There was a problem hiding this comment.
I think it is worth putting a comment at the top of the quad port section explaining what a quad port S10 RAM is. It is really simple quad port (2 read ports, 2 write ports). So you have 4 address buses, two datain and two dataout.
It looks like when you have combinational outputs (not registered outputs) you assume portaaddr affects both portadataout and portbdata. Same for portaaddr2. We also have the portbaddr and portbaddr2 affecting both portadtaouta and portbdataout. That is overly conservative: we'll only use one address port to choose the read out data. Not sure if it is always the same one or not; vqm2blif could probably always write it on one of the portaaaddr buses for the data a side, and one of the portbaddr buses for the port b side. Then portadataout and portbdataout would only be affected by the one chosen address port each.
Right now portadatain and portbdatain are always affecting the timing of portadataout and portbdataout when the outputs aren't registered. That is also overly conservative; the datain values will be stored in the RAM but (by default at least) Quartus assumes they won't flow through to the outputs the same cycle (that's called read-during-write behaviour -- Quartus has settings to choose which one you assume, but I think we should assume the less conservative ones where dataina does not affect dataouta etc.
Suggest updating the timing section here.
This pull request adds support for quad-port RAM in the Stratix 10 architecture file. It also updates the vqm2blif tool to process quad-port RAMs from VQM files and generate the appropriate operation modes in the BLIF output. Additionally, it makes a small modification to the MLAB primitives in the architecture capture by adding the class=memory attribute, enabling VPR to correctly pack MLABs slices in to MLAB primitives.